Repository navigation
Conversation
Collaborator
Author
Collaborator
Author
|
Reproduced on release builds of main (3f3a9ff) and of #47760, with small test files:
The new tests in CI (build 137062): the red files are PR: #47800 |
…1123bb/await-through-engine
This was referenced Nov 2, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
test.extendfixture and an around hook readthenfrom what their function returns and call it (test_context_fixtures.rs:703,test_context_around.rs:172). A replacedPromise.prototype.thengets that call. Whilethenis not callable, the run fails:Fixture "x" returned without calling use()orAroundHookSetupError.then()of a returned thenable at once and does not follow a second thenable. Vitest 5.0.3 waits asawaitdoes.Fix
BunTest::awaited_promisereturns whatawait valuewaits on (specPromiseResolve). Both sites attach to it withJSValue::then_with_value, asrun_test_callbackdoes.test/js/bun/test/vitest-flavor.test.ts: 4 new tests and a row in 1 more, 5 fail on the base. Also all oftest/js/bun/test/. Self-reviewed: 20 concerns raised, 16 addressed.Background
test.extendsets up a fixture value for a test. An around hook (aroundEach,aroundAll) wraps a test or a suite. Both are from bun test: Vitest compatibility #44833, in no release.then_with_valueregisters two native reactions on a promise. It does not readthen.thenread with a guard for athenthat is not callable. A replacedthen()still runs.Downsides
onCleanupcallback that throws after its test ended is now anuncaughtException, not anunhandledRejection.then()call one microtask turn later.Notes
Order and scope
JSPromise::promise_resolvemoved here from bun:test: wait on a Promise subclass through its own then() #41008.thenread at the fixture site twice.test.extendand the around hooks are in no release.thenread. With it, a builder fixture whose function returns a thenable gets the thenable itself whilePromise.prototype.thenis not callable. This PR repairs that.Behaviour changes
Each row was run on release builds of main (3f3a9ff), of #47760 (55b02d8) and of this branch. "Function" means a fixture function or an around hook.
Promise.prototype.thenis replacedthen()is called oncethenis not callableFixture "x" returned without calling use(),AroundHookSetupErrorPromise.prototype.thenis not callablethen()callawaitand Vitest 5.0.3runTest()in it fails withAroundHookSetupErrorthenis not callableawaitand Vitest 5.0.3thenis not callableonCleanupcallback throws when its fixture function settles after the test endedunhandledRejectionuncaughtExceptionasynconCleanupcallback rejects thenunhandledRejectionNot changed by this step: a thenable from a fixture function gets one
then()call, one turn later (#47760). A Promise subclass with its ownthen()is awaited through thatthen()at both sites, as on main.The route of an error
then(), which made a derived promise. An error that left a reaction rejected that promise, and nothing handled it.then_with_valuemakes no derived promise. The error is thrown out of the reaction job. It is reported when it is thrown, and the entry that runs then gets it.onCleanupcallback when teardown runs at once. That is when the fixture function settles after its test ended, or after its file ended for a file scope. The other errors that can leave the two fixture reactions take the same route. The reactions of an around hook call no user code.process.on("unhandledRejection")listener, main gives that error to the listener and the running test passes. This PR fails the running test with it. With only anuncaughtExceptionlistener it is the reverse.provide(). It is not part of this step.Not changed here
vi.waitFor,vi.waitUntiland the callback ofexpect.poll(src/runtime/test_runner/vi_wait.rs:324) keep thethenread and the direct call. Vitest 5.0.3 makes the same call inwaitForandwaitUntil(thenable.then(...)in itsdist). Athenthat is replaced or not callable still reaches these three. After this PR it is the only read oftheninsrc/runtime/test_runner.Implementation
awaitdoes not wait on is passed on, as before.awaited_promisereturnsNonefor a value that is not an object. It also returnsNonewhenPromiseResolvemade a new promise that is fulfilled at once: the value has no callablethen. Otherwise it returns the promise: the value itself when itsconstructoris thePromiseof the realm, or a new promise that followsthen().(value, wrapper). The four entries intest_context.bind.tsno longer passthis.then()on a native promise.JSPromise::promise_resolvewraps the existing binding ofJSC::JSPromise::resolvedPromise.VirtualMachine.rshad the one private declaration of it, and now calls the wrapper. There is no new C++.Measurements
Release builds of #47760 and of this branch from one build directory, linux-x64, JIT off. Method: a gdb single-step from the entry of the function to its return, at the fifth call.
Bun__ActiveFixture__setUp, the call of the fixture function included:asyncfixture function 10622 to 8516. A function that returns a thenable: about 10800 to about 8600. That pair moves by up to 70 between runs.Bun__AroundHook__setUp, the call of the hook included:asynchook 9392 to 7320, a hook that returns a thenable 8468 to 7686.then()derives.sizetext: 140288367 to 140287343, -1024 B.run_test_callbackis 2489 B on both. NewJSPromise::promise_resolve: 109 B.perfandvalgrindare not in the container, so there is no cache or branch number.Tests
All in
test/js/bun/test/vitest-flavor.test.ts:Promise.prototype.thenis replaced, or not callable, while the runner attaches.aroundEachandaroundAll.processlisteners log the event.then(), and a second thenable.vitest-flavor.test.ts201 pass.bun_test.test.ts,done-async.test.ts,concurrent.test.ts,jest-hooks.test.ts,test-error-code-done-callback.test.tsandexpect-deferred.test.ts: 98 pass.node-module-module.test.js: 80 pass. The new paths are clean withBUN_JSC_validateExceptionChecks=1.test/js/bun/test/4196 pass, 1 fail. The failure is "error snapshots" insnapshot.test.ts, which expects a TTY and fails on the release build of main too. The release build of bun:test: call then() of a returned thenable before the microtask drain, and once for a fixture #47760 fails the 5 tests above.then()inaroundEachandaroundAll, the second thenable, and the two promises with no callablethengive the results of this PR.Other open PRs in this code
src/jsc/event_loop.rs. This step adds no conflict there.Self-review
20 concerns raised, 16 addressed:
thenkeeps the result ofawait: a test pins both cases.vi.waitForis named with its reason.Not taken:
thenby its state.awaitand Vitest 5.0.3 pass it on.unhandledRejectionfor a cleanup that throws, with a catch in the reaction. The thrown error now goes where an error thrown from a job goes.[human-review] gate passed · iteration 1 · 7 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file